bugfix: Setting OMPI_MPI_THREAD_LEVEL to a value different than requested crashes #13211

abouteiller · 2025-04-25T15:05:13Z

Setting OMPI_MPI_THREAD_LEVEL to a value different than requested in MPI_Init_thread would invoke the error handler, even though it is an useful override in some threaded library use cases.

ompi/mpi/c/init_thread.c.in

ompi/mpi/c/init.c.in

ompi/mpi/c/init_thread.c.in

jsquyres · 2025-05-05T17:58:57Z

ompi/mpi/c/init_thread.c.in

+     */
+    if (NULL != (env = getenv("OMPI_MPI_THREAD_LEVEL"))) {
+        int env_required = atoi(env);
+        err_arg_required = err_arg_required ||


env_arg_required |= ...etc

My biggest concern here is that the user is required to know the valid thread level values as defined in the MPI ABI header. We should keep doing what we do right now, aka 0-3, and use a shift operation to convert it to the ABI value env_required = (1 << (10 + atoi(env)));

For now, the valid values are still [0-3] (because our internal ABI is not the same as the standardized ABI (yet?), I'll add this consideration to the comment requested by Jeff so we don't forget when the time comes.

FWIW: we plan to support both the existing OMPI ABI and the official MPI Forum MPI ABI. Hence, we'll probably want code that can handle both situations.

the important part is that we don't want the users to have to enter the ABI values for the environment variable for the thread level. We want this to remain indexed as it is today.

Hmm. Good point.

Maybe we should also accept strings? MPI_THREAD_SINGLE, MPI_THREAD_MULTIPLE, ... etc. I.e., something that doesn't depend on a numeric value.

can we defer this to when we implement ABI compat?

It might behoove us to do this now, so that it can be in 6.0.0 -- i.e., at a major version change. ABI may or may not make it into 6.0.0 (e.g., it won't be ready until -- at the earliest -- the end of this summer, and we may have already released 6.0.0 by then, in which case, ABI would be in 6.1.0).

It can't be that hard to implement to also check for string values. I'd suggest the following case-insensitive values:

mpi_thread_single and single

mpi_thread_serialized and serialized

mpi_thread_funneled and funneled

mpi_thread_multiple and multiple

Besides, didn't you make the argument that you want to check for the individual values because of ABI? You can't argue that we should do this PR because ABI is coming, but we shouldn't bother doing all the things because ABI isn't here yet. 😇

github-actions · 2025-05-12T17:43:09Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

1a0d2f7: review comments

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

github-actions · 2025-06-13T03:53:22Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

b056c23: OMPI_MPI_THREAD_LEVEL can now take 'multiple' 'MPI...

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

bosilca · 2025-06-13T14:51:43Z

Hold on a little on this one, I have a fancier solution I just didn't had the time to integrate it. The code is attached below, and it allows for partial matching for as long as the match is unique. As an example "s" will not be accepted because single and serialized, but si will be.

int check_env_value(const char** valid_prepositions, const char** keywords, int nb_keywords, const char* value)
{
    char *prep = NULL, *token = (char*)value  /* full match */;
    int pidx = 0, v = strtol(value, &prep, 10), found = -1;
    if ((0 == v) && (prep != value))
        return v;
    while(NULL != (prep = (char*)valid_prepositions[pidx])) {
        if( 0 == strncasecmp(prep, value, strlen(prep)) ) {
            token = value + strlen(prep);
            break;  /* got a token let's find a match */
        }
        pidx++;
    }

    for(int i = 0; i < nb_keywords; i++) {
        if( 0 == strncasecmp(keywords[i], token, strlen(token)) ) {
            if( -1 != found ) {  /* not the first match, bail out */
                return -1;
            }
            found = i;
        }
    }
    return found;
}

static const char* keywords[] = {"single", "serialized", "funneled", "multiple"};
static const char* prepositions[] = {"mpi_thread_", "thread_", NULL};

abouteiller · 2025-06-13T18:58:05Z

I think you have perms to push in my branch if you want to do that

jsquyres

I think we're going to need to get this documented, too. It's cool new functionality, but if it doesn't appear in a man page and/or some other docs, no one will ever use it.

`requested` in `MPI_Init_thread` would invoke the error handler, even though it is an useful override in some threaded library use cases. Signed-off-by: Aurelien Bouteiller <[email protected]>

(single,etc) in addition to numeric 0-3 values Signed-off-by: Aurelien Bouteiller <[email protected]>

This function support prepositions (such as mpi_thread_) and partial matching (such as "fun" for funnelled). Signed-off-by: George Bosilca <[email protected]>

jsquyres

I took the liberty of updating the MPI_Init* man pages. You're welcome. 😇

I made minor changes to some MPI_Session_* man pages, too, but most changes were to

…ages Including, but not limited to: * Added much more description of and distinction between the MPI world model and the MPI session model. Updated a lot of old, pre-MPI-world-model/pre-MPI-session-model text that was now stale / outdated, especially in the following pages: * MPI_Init(3), MPI_Init_thread(3) * MPI_Initialized(3) * MPI_Finalize(3) * MPI_Finalized(3) * MPI_Session_init(3) * MPI_Session_finalize(3) * Numerous formatting updates * Slightly improve the C code examples * Describe the mathematical relationship between the various MPI_THREAD_* constants in MPI_Init_thread(3) * Note that the mathematical relationships render nicely in HTML, but don't render entirely properly in nroff. This commit author is of the opinion that the nroff rendering is currently "good enough", and some Sphinx maintainer will fix it someday. * Add descriptions about the $OMPI_MPI_THREAD_LEVEL env variable and how it is used in MPI_Init_thread(3) * Added more seealso links Signed-off-by: Jeff Squyres <[email protected]>

jsquyres · 2025-06-22T12:48:31Z

It occurred to me overnight that the updates I made in MPI_Init*(3) and MPI_Finalize(3) were indicative of the fact that none of our man pages had been updated to reflect the fact that the world model now exists (and is distinct from the MPI session model). So I updated even more text this morning to describe and clarify the MPI world model vs. the MPI session model.

I.e., in documenting the MPI_THREAD_* constant-setting functionality via the *OMPI_MPI_THREAD_LEVEL env variable, this PR became an excuse the update the man pages about the MPI world model vs. the MPI session model.

Reviewers should read these man pages:

abouteiller added the bug label Apr 25, 2025

abouteiller self-assigned this Apr 25, 2025

abouteiller linked an issue Apr 25, 2025 that may be closed by this pull request

OMPI_MPI_THREAD_LEVEL cannot override 'required' in MPI_Init_thread #13210

Open

github-actions bot added the Target: main label Apr 25, 2025

abouteiller commented Apr 25, 2025

View reviewed changes

ompi/mpi/c/init_thread.c.in Show resolved Hide resolved

abouteiller removed the bug label Apr 25, 2025

abouteiller force-pushed the bugfix/env-thread-level-ignored branch 4 times, most recently from b71461f to b481893 Compare April 25, 2025 16:29

abouteiller requested review from jsquyres and bosilca May 5, 2025 15:33

jsquyres reviewed May 5, 2025

View reviewed changes

abouteiller force-pushed the bugfix/env-thread-level-ignored branch from b481893 to 1a0d2f7 Compare May 12, 2025 17:42

abouteiller force-pushed the bugfix/env-thread-level-ignored branch 2 times, most recently from 8ad7579 to fe5d20e Compare May 15, 2025 15:55

abouteiller force-pushed the bugfix/env-thread-level-ignored branch from b056c23 to 5059134 Compare June 13, 2025 03:57

jsquyres requested changes Jun 14, 2025

View reviewed changes

jsquyres mentioned this pull request Jun 21, 2025

fix some memleak #13306

Open

abouteiller and others added 3 commits June 21, 2025 15:38

bugfix: Setting OMPI_MPI_THREAD_LEVEL to a value different than

27332fc

`requested` in `MPI_Init_thread` would invoke the error handler, even though it is an useful override in some threaded library use cases. Signed-off-by: Aurelien Bouteiller <[email protected]>

OMPI_MPI_THREAD_LEVEL can now take 'multiple' 'MPI_THREAD_MULTIPLE'

3de2489

(single,etc) in addition to numeric 0-3 values Signed-off-by: Aurelien Bouteiller <[email protected]>

A slightly more flexible way to select the thread level.

9b24edc

This function support prepositions (such as mpi_thread_) and partial matching (such as "fun" for funnelled). Signed-off-by: George Bosilca <[email protected]>

jsquyres force-pushed the bugfix/env-thread-level-ignored branch from 54682e4 to 0f4673c Compare June 21, 2025 19:39

jsquyres previously approved these changes Jun 21, 2025

View reviewed changes

jsquyres dismissed their stale review via 027fda5 June 21, 2025 19:48

jsquyres force-pushed the bugfix/env-thread-level-ignored branch from 0f4673c to 027fda5 Compare June 21, 2025 19:48

jsquyres previously approved these changes Jun 21, 2025

View reviewed changes

jsquyres dismissed their stale review via 1e66d4c June 22, 2025 12:45

jsquyres force-pushed the bugfix/env-thread-level-ignored branch from 027fda5 to 1e66d4c Compare June 22, 2025 12:45

jsquyres approved these changes Jun 22, 2025

View reviewed changes

bugfix: Setting OMPI_MPI_THREAD_LEVEL to a value different than requested crashes #13211

Are you sure you want to change the base?

bugfix: Setting OMPI_MPI_THREAD_LEVEL to a value different than requested crashes #13211

Uh oh!

Conversation

abouteiller commented Apr 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 12, 2025

Uh oh!

github-actions bot commented Jun 13, 2025

Uh oh!

bosilca commented Jun 13, 2025

Uh oh!

abouteiller commented Jun 13, 2025

Uh oh!

jsquyres left a comment

Choose a reason for hiding this comment

Uh oh!

jsquyres left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsquyres commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jsquyres left a comment •

edited

Loading

jsquyres commented Jun 22, 2025 •

edited

Loading